JP-3658: JWST Accommodation of the Ramp Fitting Likelihood Algorithm #8631
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8631 +/- ##
==========================================
- Coverage 61.75% 61.75% -0.01%
==========================================
Files 377 377
Lines 38750 38755 +5
==========================================
+ Hits 23931 23934 +3
- Misses 14819 14821 +2 ☔ View full report in Codecov by Sentry. |
kmacdonald-stsci
force-pushed
the
jp_3658
branch
from
ff96356
to
8d19c64
Compare
kmacdonald-stsci
force-pushed
the
jp_3658
branch
from
8d19c64
to
d831d80
Compare
melanieclarke
changed the title
There was a problem hiding this comment.
This needs to wait until the stcal PR is merged:
spacetelescope/stcal#278
In the meantime, a few comments on the structure and log messages here. Also, we'll need to update the documentation here, to match the changes in stcal and add the new option for the algorithm argument.
|
Looking better, thanks! It looks like we still need documentation updates for the ramp fit step, though. |
|
Testing on some real data with ngroups=3, it looks like it warns and switches to OLS_C correctly when the step is called from the detector1 pipeline. When ramp_fit is called directly, though, it reports Sorry for the late realization, but since we are no longer turning off jump detection by default, perhaps this check should actually just be moved from the detector1 pipeline to the ramp fit step instead, so we can handle both calling conditions in the same place. |
… the detector1 pipeline to handle any problems that can be encountered using the likelihood algorithm.
… automatically skipping the normal pipeline jump detection algorithm.
kmacdonald-stsci
force-pushed
the
jp_3658
branch
from
50e3096
to
bd27d2d
Compare
|
@kmacdonald-stsci - coming back to this one, now that the stcal PR is close to ready. Can we move the check for ngroups into the ramp_fit step and add some likelihood documentation? |
If you want the |
It looks to me like the |
…KELY algorithm for ramp fitting.
jwst/ramp_fitting/ramp_fit_step.py
Outdated
| log.info(f"When selecting the LIKELY ramp fitting algorithm the" | ||
| " ngroups needs to be a minimum of {likely_fit.LIKELY_MIN_NGROUPS}," | ||
| " but ngroups = {ngroups}. Due to this, the ramp fitting algorithm" | ||
| " is being changed to OLS_C") |
There was a problem hiding this comment.
This message is long - let's split it up into two messages and make them warnings. Also, the f needs to go at the front of all the substrings to get the variables in there properly.
| log.info(f"When selecting the LIKELY ramp fitting algorithm the" | |
| " ngroups needs to be a minimum of {likely_fit.LIKELY_MIN_NGROUPS}," | |
| " but ngroups = {ngroups}. Due to this, the ramp fitting algorithm" | |
| " is being changed to OLS_C") | |
| log.warning(f"When selecting the LIKELY ramp fitting algorithm the" | |
| f" ngroups needs to be a minimum of {LIKELY_MIN_NGROUPS}," | |
| f" but ngroups = {ngroups}.") | |
| log.warning("Due to this, the ramp fitting algorithm" | |
| " is being changed to OLS_C.") |
There was a problem hiding this comment.
Thanks for fixing the variable reference. Can you please also add the f at the beginning of the substrings, so the variables are printed properly? Also, split into two messages and make it a warning instead of an info.
|
EDIT: Scratch that, pin suggestion being taken care of in separate PR. I think I can take it from here. |
Resolves JP-3658
Closes #
This PR addresses the use of the likelihood algorithm with ramp fitting. This updates the
specfor the arguments of ramp fitting to allow forLIKELYas an algorithm option. Also, the detector1 pipeline is modified to do some special handling needed when using the likelihood algorithm. In particular, there data needs at least 4 groups per integration and, since the likelihood algorithm does its own jump detection, the normal pipeline jump detection needs to be skipped when using the likelihood algorithm.Checklist for PR authors (skip items if you don't have permissions or they are not applicable)
CHANGES.rstwithin the relevant release sectionHow to run regression tests on a PR